Skip to content
This repository has been archived by the owner on Dec 20, 2019. It is now read-only.

Bug/BLE packet parsing #91

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

scheunemann
Copy link

@scheunemann scheunemann commented Jun 27, 2017

Running [email protected] and [email protected] on Linux 4.11.6-3-ARCH #1 SMP PREEMPT Thu Jun 22 12:21:46 CEST 2017 x86_64 GNU/Linux.

Issue

Hence the long package answers, it seems only notable with collission detection. Sometimes, a long list of sync losses apears and collsion detection stucks since waiting for sync responses. There are two main causes for that.

Cause 1

A typical collsion detection package could have 17 bytes, which can get split over two BLE packages like this:

$ btmon
> ACL Data RX: Handle 3585 flags 0x02 dlen 27              #137 [hci0] 5.921519
  ATT: Handle Value Notification (0x1b) len 22
    Handle: 0x0010
      Data: fffe070011006afc3e0000020016002200000193
> ACL Data RX: Handle 3585 flags 0x02 dlen 9               #138 [hci0] 5.922144
  ATT: Handle Value Notification (0x1b) len 4
    Handle: 0x0010
      Data: c8ad

At two positions it is assumed that the minimal package size is at least 6 bytes (https://github.com/orbotix/sphero.js/blob/master/lib/adaptors/ble.js#L168 and https://github.com/orbotix/sphero.js/blob/master/lib/packet.js#L85)

The last two bytes get eaten and instead, another valid package got attached and will become invalid making the whole collision detection inapplicable.

Solution 1

Except all package length and use a different parsing idea https://github.com/orbotix/sphero.js/blob/master/lib/packet.js#L71.

Basically, the core still remains with the "checkIfValid" function. However, smaller packets get stored for further use. Thus, a new function is needed checking whether a small package / concatenated package is for sure invalid.

For example, the inital sync has for some reason the package 753e to offer, which get's dropped.

$ btmon 
> ACL Data RX: Handle 3585 flags 0x02 dlen 5              #111 [hci0] 28.345319
      ATT: Write Response (0x13) len 0
> ACL Data RX: Handle 3585 flags 0x02 dlen 9              #112 [hci0] 28.345912
      ATT: Handle Value Notification (0x1b) len 4
        Handle: 0x0010
          Data: 753e
< ACL Data TX: Handle 3585 flags 0x00 dlen 20             #113 [hci0] 28.352661
      ATT: Write Command (0x52) len 15
        Handle: 0x000e
          Data: ffff0212000701202020200162
> HCI Event: Number of Completed Packets (0x13) plen 5    #114 [hci0] 28.361244
        Num handles: 1
        Handle: 3585
        Count: 1
> ACL Data RX: Handle 3585 flags 0x02 dlen 13             #115 [hci0] 28.390439
      ATT: Handle Value Notification (0x1b) len 8
        Handle: 0x0010
          Data: ffff000001fe

Cause 2

Two sync responses can be packed in one BLE package like this: ff ff 00 42 01 bc ff ff 00 43 01 bb.

Computing the first package and only compute the rest bytes in the next response results again in a chain of sync losses.

Solution 2

If the received (fresh) buffer is valid (in the meaning of the old parse method) compute this and drop the temporal buffer (the old package). Thus, only one sync loss appears. From what I can see, it needs changes in the whole parsing infrastructure to implement a clean solution.

… notifications" are of 22 bytes, so they may come in two separate packages of 24 and 4 bytes (containing sphero.js packages of 20 and 2 bytes)
… are of minimal length (6 bytes) and have not a valid header.
…size correct + some extra bytes) or unvalid (proper header for fragments >= 2 bytes)

Change logic, drop first package if two valid packages are received
@scheunemann
Copy link
Author

Regarding the failed test: "Packet #parse with sync response SOPs don't pass validation and @partialBuffer is emptytialBuffer should not be empty" is a described case in the request. If ignored, that's the only fail.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant